devpkg: auto-patch python #2250
Conversation
internal/devpkg/package.go
Outdated
|
|
||
| func (p *Package) PatchGlibc() bool { | ||
| if p.CanonicalName() == "python" { | ||
| if p.patchGlibc != nil && !p.patchGlibc() { |
There was a problem hiding this comment.
I think this actually still needs a tweak to distinguish between missing and false.
There was a problem hiding this comment.
Yeah, looks like function returns false in both cases https://github.com/jetify-com/devbox/blob/main/internal/devpkg/package.go#L113C1-L115C5
|
|
Should we add a feature flag for the opt out? Or just go for it? |
|
I would suggest that we do a beta release, test it a bunch internally + with the community, and then release. I think an opt-in/flag is too hard for users to discover and activate. I would be open to an opt-out, maybe with a differently named flag. |
That's probably a good idea since it does a bit more now. What about
I haven't tested it on any other packages, but the patching logic is pretty general. It'll look for any ELF binaries in the package's The stuff with restoring build inputs is specific to Python and will just be a no-op on other packages. |
|
Prefer |
gcurtis
force-pushed
the
gcurtis/patchpkg-auto
branch
from
465f12b to
f7bb72b
Compare
|
@savil @mikeland73 I made some pretty substantial changes if you want to take another look. I switched things over to use a new |
Add a `devbox add --patch <mode>` flag that replaces `--patch-glibc` and
a corresponding `patch` JSON field that replaces `patch_glibc`. The new
name reflects the new patching behavior, which affects more than just
glibc.
The new patch flag/field is a string instead of a bool. Valid values are
`auto`, `always` and `never`. The default is `auto`.
devbox add --patch <auto/always/never>
- `auto` - let Devbox decide if a package should be patched. Currently
only enables patching for Python or if `patch_glibc` is true in the
config.
- `always` - always attempt to patch a package. Corresponds to the
`--patch-glibc=true` behavior.
- `never` - never patch a package, even if `auto` would.
If a config has an existing package with the `"patch_glibc": true`
field, it's interpreted as `"patch": "always"` but the config itself
isn't modified. However, if the user runs a command that does write to
the config, then `patch_glibc` will be migrated to `patch`.
Example `devbox.json`:
```json5
{
"packages": {
"ruby": {
"version": "latest",
"patch": "always"
}
"python": {
"version": "latest"
// No patch field implies "auto".
// "patch": "auto"
}
}
}
```
gcurtis
force-pushed
the
gcurtis/patchpkg-auto
branch
from
f7bb72b to
ae97e3f
Compare
|
|
||
| func (c *configAST) FindPkgObject(name string) *hujson.Object { | ||
| // removePatch removes the patch field from the named package. | ||
| func (c *configAST) removePatch(name string) { |
There was a problem hiding this comment.
Could we generalize these similar to setPackageBool ?
removePackageField for removing.
setPackageString for adding.
setPatch could wrap these (remove patch_glibc add patch) or you could not special case at all and have the code in packages.go do that logic). That keeps the ast more logic agnostic.
Honestly, you could also not remove patch_glibc. It doesn't really do harm and would allow us to have simpler code.
There was a problem hiding this comment.
Making them more general sounds good to me. I'd like to do in a follow-up PR though just to get the patching stuff out the door.
Add a
devbox add --patch <mode>flag that replaces--patch-glibcand a correspondingpatchJSON field that replacespatch_glibc. The new name reflects the new patching behavior, which affects more than just glibc.The new patch flag/field is a string instead of a bool. Valid values are
auto,alwaysandnever. The default isauto.auto- let Devbox decide if a package should be patched. Currently only enables patching for Python or ifpatch_glibcis true in the config.always- always attempt to patch a package. Corresponds to the--patch-glibc=truebehavior.never- never patch a package, even ifautowould.If a config has an existing package with the
"patch_glibc": truefield, it's interpreted as"patch": "always"but the config itself isn't modified. However, if the user runs a command that does write to the config, thenpatch_glibcwill be migrated topatch.Example
devbox.json: